async-hooks: introduce async-storage API #26540

vdeturckheim · 2019-03-09T10:07:51Z

This introduces a new API in core to provide asynchronous storage features.

More and more Node.js users have been needing such API to:

build monitoring tools
build features that enhence code readability and reduce the need of passing HTTP request objects all the way to one single method that needs it (for instance, a logger).

Some userland modules exist but they:

sometimes badly interact with each other
introduce potential easy mistakes in usage (need to be required first)
might need some specific addons to handle part of the ecosystem properly

Havin such API in core will make the ecosystem more stable and reliable when needing an asynchronous storage.

This is still an early work and there is probably a lot of updates to do to this PR :)

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

lib/async_storage.js

mscdex

IMO this is best left as a userland module, especially considering it can be fairly easily implemented using just async_hooks. There are also some assumptions being made design-wise here that others might disagree with and would want more flexibility.

Even if this kind of feature were to be added to node core, it would be better to have it start in userland to allow easier iteration on the idea first, to figure out the best API and behaviors between all parties that would be involved.

doc/api/async_storage.md

lib/async_storage.js

addaleax · 2019-03-09T11:08:55Z

@mscdex This was talked about at the diagnostics summit. I don’t think the feature has to exist in this particular form, but having it in core made sense in general, and in particular performance-wise, since a good implementation could allow us to skip calling async_hooks callbacks.

FYI @nodejs/tsc

mcollina · 2019-03-09T11:42:33Z

I think this functionality belongs in core. We should offer a single way to do cls that is high performant, stable and reliable. It’s not an ecosystem concern, it’s a runtime concern (as much as a ThreadLocal implementation is for Java/.NET/.. etc).

jasnell · 2019-03-09T11:48:01Z

doc/api/async_storage.md

@@ -0,0 +1,86 @@
+# Async Storage
+
+<!--introduced_in=v12.x.x-->


is this supposed to be added: REPLACEME ? Does introduced_in work also?

I think it’s a separate system used for the per-page “View another version” dropdown in the generated documentation:

jasnell · 2019-03-09T11:54:39Z

I agree that this should be in core and there was consensus among the @nodejs/diagnostics WG members attending the diagnostics summit. Sure, we can bikeshed around the specific API, and I would like to get some of the APM vendors to review this before it lands, it would be generally better to have a single authoritative API and implementation of this than multiple userland implementations so that APM solutions built on top can be implemented consistently.

vsemozhetbyt · 2019-03-09T12:09:21Z

Should we create npm placeholder for this name as per our guide?

mcollina · 2019-03-09T12:21:31Z

Go ahead. I kind of expect this to go under `@nodejs/async-storage` but better safe than sorry. Il giorno sab 9 mar 2019 alle ore 13:10 Vse Mozhet Byt < notifications@github.com> ha scritto:

…

Should we create npm placeholder for this name as per our guides? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#26540 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADL4xdIdUUD6v4zX5DYe6C_E1y5GCinks5vU6SmgaJpZM4bmmI4> .

vsemozhetbyt · 2019-03-09T12:28:11Z

Sorry, I am not experienced in registering placeholders in npm, so anybody more confident please do it.

mcollina · 2019-03-09T12:39:02Z

There is https://www.npmjs.com/package/async-storage, which makes calling this module async_storage not a great idea. @nodejs/async_storage is ok.

jasnell · 2019-03-09T12:44:01Z

Does this require a separate new top level module? Could this be exposed alternatively through the existing async_hooks module? Or via util?

vdeturckheim · 2019-03-09T14:13:41Z

@jasnell I actually think it would make sense to have that in util too. I am just worried potential future features would make this API big enough for us to regret it was not a module. That said, I'd be happy to move this to another module.

vdeturckheim · 2019-03-09T14:17:14Z

I will update the PR over the week end and start to contact APM and RASP vendors to ensure thay can comment.

Thanks a lot to all of you for the comments and suggestions!

devsnek · 2019-03-09T14:20:36Z

if this is going to be in core I'd want it to be more similar to https://github.com/WICG/kv-storage

jasnell · 2019-03-09T14:22:46Z

@vdeturckheim ... One thing to consider is that by putting this into a new module, it automatically becomes semver-major and significantly harder to backport. My hope is that we could at least get this into 10.x also.

vdeturckheim · 2019-03-09T14:25:52Z

@jasnell good point, I'll move it to util then (this is a killer argument for me indeed ;) )

@devsnek I did not know about this proposal. I'd be interested in other opinions here but the solutions I could see would probably be a bit slow.

devsnek · 2019-03-09T14:29:45Z

@vdeturckheim reading over this again it seems i was confused as to what this pr is adding to node.

i think it might lend itself better to a different name like "async_closure" or "async_context" something.

i'd be strongly in favor of making the class available as an async_hooks.Context

targos · 2019-03-09T14:31:32Z

I think it would be better placed in async_hooks instead of util.

othiym23 · 2019-03-09T17:11:01Z

As a developer and co-maintainer of continuation-local-storage, I’m very happy to see this functionality coming to Node core, and am amused to see how positive the reception is this time. A lot can change in 6 years. If anyone wants to reuse the name, or join this with the existing continuation-local-storage, I’m happy to hand the name over to the Foundation, although I understand that this implementation is simpler to use than CLS.

Notable changes: * async_hooks * introduce async-context API (vdeturckheim) #26540 * stream * support passing generator functions into pipeline() (Robert Nagy) #31223 * tls * expose SSL\_export\_keying\_material (simon) #31814 * vm * implement vm.measureMemory() for per-context memory measurement (Joyee Cheung) #31824 PR-URL: #32027

Sytten · 2020-03-06T07:28:57Z

Any chance this will be backported to v12?

puzpuzpuz · 2020-03-06T09:04:51Z

Any chance this will be backported to v12?

It would be great to have AsyncLocalStorage in v12, so I'm all for backporting it.

Note: executionAsyncResource (#30959) has to be backported first.

cc @Qard @vdeturckheim

Qard · 2020-03-06T09:07:54Z

As far as I know, backporting executionAsyncResource to v12 should not be a problem, but I haven't tested it myself.

Flarna · 2020-03-06T09:26:17Z

Care must be taken to include also the followup PRs fixing various bugs found afterwards.

puzpuzpuz · 2020-03-06T19:04:55Z

I've created a backport PR for executionAsyncResource: #32131. Hopefully, it includes all bugfixes.

codebytere · 2020-03-16T01:16:06Z

@puzpuzpuz thanks! this next release is a patch so it'll go out in the subsequent minor.

puzpuzpuz · 2020-03-17T06:31:15Z

v12 backport: #32318

Adding AsyncLocalStorage class to async_hooks module. This API provide a simple CLS-like set of features. Co-authored-by: Andrey Pechkurov <apechkurov@gmail.com> PR-URL: nodejs#26540 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Adding AsyncLocalStorage class to async_hooks module. This API provide a simple CLS-like set of features. Co-authored-by: Andrey Pechkurov <apechkurov@gmail.com> PR-URL: #26540 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

indutny · 2020-07-16T18:33:18Z

Providing a reference to another issue here because most interested parties are subscribed to this issue.

vdeturckheim requested review from mcollina, jasnell, misterdjules and bmeurer March 9, 2019 10:07

This comment has been minimized.

Sign in to view

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Mar 9, 2019

mcollina reviewed Mar 9, 2019

View reviewed changes

lib/async_storage.js Outdated Show resolved Hide resolved

lib/async_storage.js Outdated Show resolved Hide resolved

lib/async_storage.js Outdated Show resolved Hide resolved

lib/async_storage.js Outdated Show resolved Hide resolved

mscdex previously requested changes Mar 9, 2019

View reviewed changes

addaleax reviewed Mar 9, 2019

View reviewed changes

doc/api/async_storage.md Outdated Show resolved Hide resolved

doc/api/async_storage.md Outdated Show resolved Hide resolved

doc/api/async_storage.md Outdated Show resolved Hide resolved

lib/async_storage.js Outdated Show resolved Hide resolved

jasnell reviewed Mar 9, 2019

View reviewed changes

vsemozhetbyt added the tsc-review label Mar 9, 2019

devsnek added the async_hooks Issues and PRs related to the async hooks subsystem. label Mar 9, 2019

Trott removed the tsc-review label Mar 10, 2019

vdeturckheim changed the title ~~async-storage: introduce async-storage API~~ async-hooks: introduce async-storage API Mar 10, 2019

addaleax added the notable-change PRs with changes that should be highlighted in changelogs. label Mar 1, 2020

This was referenced Mar 16, 2020

test: add GC test for disabled AsyncLocalStorage #31995

Closed

async_hooks: fix context loss after nested calls to AsyncLocalStorage #32085

Closed

puzpuzpuz mentioned this pull request Mar 17, 2020

[v12.x backport] async_hooks: add AsyncLocalStorage #32318

Closed

4 tasks

targos added the backported-to-v12.x label Mar 17, 2020

legendecas mentioned this pull request Mar 20, 2020

Deprecate destroy hook of async_hooks #32386

Closed

wentout mentioned this pull request Apr 9, 2020

async_hooks performance/stability improvement plans nodejs/diagnostics#376

Closed

targos mentioned this pull request May 2, 2020

v12.17.0 release proposal #33197

Merged

legendecas mentioned this pull request May 27, 2020

Avoid name conflict with Web LocalStorage API tc39/proposal-async-context#1

Closed

vdeturckheim mentioned this pull request May 27, 2020

AsyncLocalStorage potential issue ? #33580

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

async-hooks: introduce async-storage API #26540

async-hooks: introduce async-storage API #26540

vdeturckheim commented Mar 9, 2019

This comment has been minimized.

mscdex left a comment

addaleax commented Mar 9, 2019

mcollina commented Mar 9, 2019

jasnell Mar 9, 2019

addaleax Mar 9, 2019

jasnell commented Mar 9, 2019

vsemozhetbyt commented Mar 9, 2019 •

edited

Loading

mcollina commented Mar 9, 2019 via email

vsemozhetbyt commented Mar 9, 2019 •

edited

Loading

mcollina commented Mar 9, 2019 •

edited

Loading

jasnell commented Mar 9, 2019

vdeturckheim commented Mar 9, 2019

vdeturckheim commented Mar 9, 2019

devsnek commented Mar 9, 2019

jasnell commented Mar 9, 2019

vdeturckheim commented Mar 9, 2019

devsnek commented Mar 9, 2019 •

edited

Loading

targos commented Mar 9, 2019

othiym23 commented Mar 9, 2019

Sytten commented Mar 6, 2020

puzpuzpuz commented Mar 6, 2020

Qard commented Mar 6, 2020

Flarna commented Mar 6, 2020

puzpuzpuz commented Mar 6, 2020

codebytere commented Mar 16, 2020

puzpuzpuz commented Mar 17, 2020

indutny commented Jul 16, 2020 •

edited

Loading

async-hooks: introduce async-storage API #26540

async-hooks: introduce async-storage API #26540

Conversation

vdeturckheim commented Mar 9, 2019

Checklist

This comment has been minimized.

mscdex left a comment

Choose a reason for hiding this comment

addaleax commented Mar 9, 2019

mcollina commented Mar 9, 2019

jasnell Mar 9, 2019

Choose a reason for hiding this comment

addaleax Mar 9, 2019

Choose a reason for hiding this comment

jasnell commented Mar 9, 2019

vsemozhetbyt commented Mar 9, 2019 • edited Loading

mcollina commented Mar 9, 2019 via email

vsemozhetbyt commented Mar 9, 2019 • edited Loading

mcollina commented Mar 9, 2019 • edited Loading

jasnell commented Mar 9, 2019

vdeturckheim commented Mar 9, 2019

vdeturckheim commented Mar 9, 2019

devsnek commented Mar 9, 2019

jasnell commented Mar 9, 2019

vdeturckheim commented Mar 9, 2019

devsnek commented Mar 9, 2019 • edited Loading

targos commented Mar 9, 2019

othiym23 commented Mar 9, 2019

Sytten commented Mar 6, 2020

puzpuzpuz commented Mar 6, 2020

Qard commented Mar 6, 2020

Flarna commented Mar 6, 2020

puzpuzpuz commented Mar 6, 2020

codebytere commented Mar 16, 2020

puzpuzpuz commented Mar 17, 2020

indutny commented Jul 16, 2020 • edited Loading

vsemozhetbyt commented Mar 9, 2019 •

edited

Loading

vsemozhetbyt commented Mar 9, 2019 •

edited

Loading

mcollina commented Mar 9, 2019 •

edited

Loading

devsnek commented Mar 9, 2019 •

edited

Loading

indutny commented Jul 16, 2020 •

edited

Loading